8365192: post_meth_exit should be in vm state when calling get_jvmti_thread_state #26713
Conversation
|
👋 Welcome back lmesnik! A progress list of the required criteria for merging this PR into |
|
@lmesnik This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 48 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
openjdk
bot
added
serviceability
Webrevs
|
|
I ran tier1-5 testing and separately verified that test pass with jvmti strass agent. |
| JavaThread* current = thread; // for JRT_BLOCK | ||
| JRT_BLOCK | ||
| state = get_jvmti_thread_state(thread); | ||
| JRT_BLOCK_END |
There was a problem hiding this comment.
The JRT_BLOCK is defined as:
#define JRT_BLOCK \
{ \
assert(current == JavaThread::current(), "Must be"); \
ThreadInVMfromJava __tiv(current); \
JavaThread* THREAD = current; /* For exception macros. */ \
DEBUG_ONLY(VMEntryWrapper __vew;)
I'd suggest something like this instead of using JRT_BLOCK:
- JvmtiThreadState *state = get_jvmti_thread_state(thread);
+ JvmtiThreadState *state = nullptr;
+ {
+ ThreadInVMfromJava __tiv(thread);
+ state = get_jvmti_thread_state(thread);
+ }
Alternatively, the JRT_BLOCK can be started at the line 1837 and ended with JRT_BLOCK_END at the line 1875. Not sure, what issue we can encounter with this though. At least, it is worth a try.
There was a problem hiding this comment.
As I understand the post_method_entry was called via JRT_BLOCK_ENTRY and not JRT_BLOCK by the reason. We need to be in Java. See comments for the method invocation.
// This is a JRT_BLOCK_ENTRY because we have to stash away the return oop
// before transitioning to VM, and restore it after transitioning back
// to Java. The return oop at the top-of-stack, is not walked by the GC.
JRT_BLOCK_ENTRY(void, InterpreterRuntime::post_method_exit(JavaThread* current))
LastFrameAccessor last_frame(current);
JvmtiExport::post_method_exit(current, last_frame.method(), last_frame.get_frame());
JRT_END
And thanks for simplification, it is a good idea. I've updated the PR.
I am running tier1-8 for Hotspot tests to ensure that nothing is broken.
Okay, thanks. I'd also run the tier 6 to be more safe. |
|
|
||
| JvmtiThreadState *state = nullptr; | ||
| { | ||
| ThreadInVMfromJava __tiv(thread); |
There was a problem hiding this comment.
Nit: Maybe rename: __tiv => tiv. The prefix __ is normally used in macros to avoid potential naming conflicts.
There was a problem hiding this comment.
Thank you for the update on this!
| JvmtiThreadState *state = nullptr; | ||
| { | ||
| ThreadInVMfromJava tiv(thread); | ||
| state = get_jvmti_thread_state(thread); |
There was a problem hiding this comment.
The issue I see is that get_jvmti_thread_state() can safepoint for virtual threads (and now also for platform threads because of ~ThreadInVMfromJava), which brings us back to the bug 8255452 was trying to fix: if there is a return oop at the top of the stack, it could become invalid if a GC occurs. I think we will have to unconditionally save the return value in case it's an oop, before doing anything else.
The method
get_jvmti_thread_state()
should be called only while thread is in vm state.
The post_method_exit is doing some preparation before switching to vm state. This cause issues if thread is needed to initialize jvmti thread state.
The fix was found using jvmti stress agent and thus no additional regression test is required.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26713/head:pull/26713$ git checkout pull/26713Update a local copy of the PR:
$ git checkout pull/26713$ git pull https://git.openjdk.org/jdk.git pull/26713/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 26713View PR using the GUI difftool:
$ git pr show -t 26713Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26713.diff
Using Webrev
Link to Webrev Comment